execinfra: extract out some packages to break dependencies #79938
Conversation
|
This change is |
This commit refactors `execinfra` package by extracting some things in order to break the dependency of `colexecop` on it. Two main pieces had to be refactored: - `stats.go` file is now moved into `execstats` since it seems to be a much more suitable place. This required inlining `GetTraceData` function helper and breaking a dependency of `ShouldCollectStats` helper on `execinfra.FlowCtx`. - `operator.go` which defines `OpNode` interface (implemented by both columnar operators and row-by-row processors) has been moved (and renamed to `op_node.go`) to a new package `execopnode`. Release note: None
yuzefovich
force-pushed
the
colexecop-execinfra-dep
branch
from
bbe9bfe to
54aac03
Compare
This commit extracts `execinfra.Releasable` interface into a new `execreleasable` package in order to break the dependency of several packages of the vectorized engine on `execinfra`. This will allow those packages (e.g. `colexecproj`) to start being built sooner which will shorten the build time. Release note: None
yuzefovich
force-pushed
the
colexecop-execinfra-dep
branch
from
54aac03 to
7a3b7e8
Compare
|
This PR makes it so that the vectorized engine packages are no longer on the critical path. |
There was a problem hiding this comment.
Keep these build time improvement PRs coming!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @michae2)
There was a problem hiding this comment.
Reviewed 8 of 73 files at r1, 3 of 41 files at r2.
Reviewable status:
pkg/sql/colexec/builtin_funcs.go, line 42 at r2 (raw file):
var _ colexecop.Operator = &defaultBuiltinFuncOperator{} var _ execreleasable.Releasable = &defaultBuiltinFuncOperator{}
nit: is there any other name that would also make sense to change it to? Just thinking of something that would not have this usage of releasable.Releasable
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/builtin_funcs.go, line 42 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: is there any other name that would also make sense to change it to? Just thinking of something that would not have this usage of
releasable.Releasable
Yeah, I'm not fond of it either but couldn't come up with anything better. My only other ideas were execinfrabase of execbase, but IMO those are even worse. Any other ideas?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/builtin_funcs.go, line 42 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Yeah, I'm not fond of it either but couldn't come up with anything better. My only other ideas were
execinfrabaseofexecbase, but IMO those are even worse. Any other ideas?
I can't think of anything either, so let's keep this way. We can always revisit later :)
|
TFTRs! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
execinfra: shrink the package down a bit
This commit refactors
execinfrapackage by extracting some things inorder to break the dependency of
colexecopon it. Two main pieces hadto be refactored:
stats.gofile is now moved intoexecstatssince it seems to bea much more suitable place. This required inlining
GetTraceDatafunction helper and breaking a dependency of
ShouldCollectStatshelperon
execinfra.FlowCtx.operator.gowhich definesOpNodeinterface (implemented by bothcolumnar operators and row-by-row processors) has been moved (and
renamed to
op_node.go) to a new packageexecopnode.Release note: None
execinfra: extract Releasable interface into a separate package
This commit extracts
execinfra.Releasableinterface into a newexecreleasablepackage in order to break the dependency of severalpackages of the vectorized engine on
execinfra. This will allow thosepackages (e.g.
colexecproj) to start being built sooner which willshorten the build time.
Addresses: #79357.
Release note: None